corrections for intialization #8
Conversation
|
@oyvindseland @Kirkevag @tto061 @DirkOlivie please check this pull requesr |
|
Hi Alok, |
|
@DirkOlivie I have not tested results after all these bugs fixed. I tested bit-wise reproducibility in between and then, I tested results in-between with and without bugs only once. They were bit-identical. But, if @tto061 and Ada has made any these tests I am not aware as they were also doing some simulation. Also, I was always executing experiment where these emissions files are not merged and I executed two simulations of 100 years and not having a single mid-month crash. If it require let me know I will do. |
tto061
requested review from
DirkOlivie,
tto061,
Kirkevag,
oyvindseland and
a team
|
This is OK to go for me. Could reviewers please approve within the next couple of days if you can, and we can merge. |
There was a problem hiding this comment.
I would suggest a review by Alf and Øyvind of the "clean-up" part of this request (the first commit).
To me it seems that the conditionals on micro_mg are necessary; and I not sure about deleting the commented code (might it be useful for future checks? is it the only code here that can be deleted? or would these deletions leave other dangling left-over code?)
There was a problem hiding this comment.
I've looked over the changes, which seem ok mostly. Exceptions
- zm_conv.F90: I assume Thomas has control of the added code, wehich is not just initializion?
- pmxsub.F90. Two variables (vnbcarr and vaitbcarr), now initialized, are also calculated, but before they are used for the first time. This is therefore a bug: the effect of this bug concerns extra diagnostics of optics at RH=0% only (for AeroCom experiments): mixture 4 and 14 has not had the right volume fraction of BC for this particular RH. The loop over higher RH values are done after calculating vnbcarr and vaitbcarr, so that seems to be OK.
|
I agree with Thomas, by the way, that the code commented out is best left as comments, for possible future use (even though it makes the code look messy). |
|
Alf, for my part I already approved the pull request (and yes it was
myself correcting that part; it has no consequences, bit-wise).
Given that, there's no point in you or others asking the first question
here.
Good for 2.; I regard all of these as bugs, because the code must not
rely on arbitrary initialisations by the compiler. It is just chance
that that does not cause errors in the calculations (as with all bad
coding).
…On 2020-06-09 11:42, Alf Kirkevåg wrote:
***@***.**** commented on this pull request.
I've looked over the changes, which seem ok mostly. Exceptions
1. zm_conv.F90: I assume Thomas has control of the added code, wehich
is not just initializion?
2. pmxsub.F90. Two variables (vnbcarr and vaitbcarr), now initialized,
are also calculated, but before they are used for the first time.
This is therefore a bug: the effect of this bug concerns extra
diagnostics of optics at RH=0% only (for AeroCom experiments):
mixture 4 and 14 has not had the right volume fraction of BC for
this particular RH. The loop over higher RH values are done after
calculating vnbcarr and vaitbcarr, so that seems to be OK.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZGLJCLUOTFYPOS6FC2Y5LRVX7WRANCNFSM4NUVK66Q>.
|
|
I just did my duty as a code reviewer, Thomas, as requested. For the second part, which of course is due to bad coding, I have now made a new github issue for this: #9 |
|
Alf, I disagree: it was not your duty but, at the contrary, a useless
and unnecessary request for a response from me.
Why?
Because it was I who nominated the reviewers, and included myself, and
immediately approved the request. I did not do that to waste my time, to
but save yours by making it clear what your role is. So you have now
forced a response from me that I did not need to give. You have not
understood that in the first place, and even now you keep insisting
without apparently considering whether you're right doing so. This does
not seem a good example of effective communication via github.
Maybe we can discuss in the meeting today how work together more
effectively via github.
…On 2020-06-09 11:55, Alf Kirkevåg wrote:
I just did my duty as a code reviewer, Thomas, as requested. For the
second part, which of course is due to bad coding, I have now made a new
github issue for this: #9 <#9>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZGLJGSWXPF2BNWSZPV35LRVYBKDANCNFSM4NUVK66Q>.
|
|
just to add, for the benefit of all those who do not understand the code in zm_conv.F90: that was EXACTLY an initialisation issue, nothing more, nothing else; just like the others (only this one had no effect on the calculations, and was caught be init=zero,arrays) |
|
Can we merge the first commit only now? I am not sure how to do this, by the way -- the automatic merge will take both commits. Any ideas? |
|
I found this command cherry-pick to take only one commit::
https://mattstauffer.com/blog/how-to-merge-only-specific-commits-from-a-pull-request/
… On 12 Jun 2020, at 14:59, tto061 ***@***.***> wrote:
Can we merge the first commit only now? I am not sure how to do this, by the way -- the automatic merge will take both commits. Any ideas?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD73WURD6OAX3ZBQ3USZZKDRWIRCJANCNFSM4NUVK66Q>.
|
|
I would imagine that you can also pick a specific commit from Alok's copy at the merging of the command line structure, but is there any specific reasons why you do not want both? |
|
This was discussed already between me, Alok and Alf; and also indirectly
at the telecon on Wednesday in the context of separating clearly
cosmetic changes from substantive changes, in the interest of
traceability and of easy diff'ing on the latter.
Right now we're not sure that the commented-out code cannot be useful
for troubleshooting purposes in the near future, nor are we sure that
that the clean-up is consistent and does not leave dangling code whose
function can be hard to understand without those commented parts.
So I'd defer this to a dedicated and more comprehensive code clean-up
effort in the near future.
…On 2020-06-12 15:23, oyvindseland wrote:
I would imagine that you can also pick a specific commit from Alok's
copy at the merging of the command line structure, but is there any
specific reasons why you do not want both?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZGLJHWGDO5W54W3MWQYLTRWIT33ANCNFSM4NUVK66Q>.
|
|
@MichaelSchulzMETNO That was the kind of things I was talking about (or trying to) during the CAM meeting; you can update user's PR to fix problems, cherry pick, etc. on behalf of the user (so that users are rewarded for their contribution and not the maintainer). |
|
OK I will try and have a go |
|
sorry, I tried. Now I'm fed up. I will use git again next month, maybe. |
|
ok , I assume the first commit is fine with everybody and I will try to cherry pick just the first commit.
lets see if I manage
Michael
… On 12 Jun 2020, at 16:08, tto061 ***@***.***> wrote:
sorry, I tried. Now I'm fed up. I will use git again next month, maybe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD73WUR7HG44DOXASQNG7R3RWIZEXANCNFSM4NUVK66Q>.
|
|
thanks, I appreciate. If you do, can you please share the git vodoo
command sequence that worked for you.
:( ( <-- me=idiot )
…On 2020-06-12 16:25, MichaelSchulzMETNO wrote:
ok , I assume the first commit is fine with everybody and I will try to
cherry pick just the first commit.
lets see if I manage
Michael
> On 12 Jun 2020, at 16:08, tto061 ***@***.***> wrote:
>
>
> sorry, I tried. Now I'm fed up. I will use git again next month, maybe.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
<#8 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD73WUR7HG44DOXASQNG7R3RWIZEXANCNFSM4NUVK66Q>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZGLJARELAY73VZWK2HBELRWI3HHANCNFSM4NUVK66Q>.
|
|
I will go back and do some git training inbetween the finishing up of the article so I can try as well but not until next week. |
|
I will merge, then revert one commit back and then we can pick the second commit later if all agree. |
|
great!
…On 2020-06-12 18:07, MichaelSchulzMETNO wrote:
I will merge, then revert one commit back and then we can pick the
second commit later if all agree.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZGLJD2XFFVG4TPGAMTI7LRWJHE3ANCNFSM4NUVK66Q>.
|
|
Might be good to test, if this is now working. I am not sure what this part if(micro_mg_version <2) then call post_proc%add_field(p(nctncons), p(packed_nctncons)) really was for. Why did Alok introduce it in the first place? hmm |
|
we did test this, but no harm in joining the big testfest before releasing.
I could test on tetralith; in fact I'd also like to still add tetralith
machine settings to CIME if possible.
…On 2020-06-12 18:16, MichaelSchulzMETNO wrote:
Might be good to test, if this is now working. I am not sure what this part
if(micro_mg_version <2) then call post_proc%add_field(p(nctncons),
p(packed_nctncons))
call post_proc%add_field(p(nctncons), p(packed_nctncons)) call
post_proc%add_field(p(nctnnbmn), p(packed_nctnnbmn))
call post_proc%add_field(p(nctnnbmn), p(packed_nctnnbmn))
ENDIF
really was for.
Why did Alok introduce it in the first place? hmm
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZGLJFQNLHU4RVKTNCS2KTRWJIGBANCNFSM4NUVK66Q>.
|
Update cam_development branch
In these files I made modifications for initialization of variables and I have initialized everything to zero. Please check it as sometime they do need to be initialized to zero they might have some other value.